Added user_roles migration, model, endpoint.#115
Conversation
test/test_helper.exs
Outdated
|
|
||
| # By default, exclude S3 tests | ||
| # To run S3 tests, run `mix test --include s3` | ||
| ExUnit.configure exclude: [:s3] |
There was a problem hiding this comment.
I'm a little concerned about making this the default. I'd like to go the other way I think, and add the exclude command in the README.
We almost want to encourage people to get their own AWS account, too, so they can actually play around with some of that if they'd like to.
What do you think? It's scary to me to rely on a default that ignores some tests.
There was a problem hiding this comment.
So the reason I made this excluded by default is that these tests should be in the minority. The majority of the time, we won't be interacting with tests involving S3 / other secret env variables. Since it's likely most people won't mess with those tests, I think it makes sense to exclude by default and call it out in the README.
Also, as long as CI is set up to include those tests, we'll be sure to catch anything before it gets merged.
If we do want to include these by default, just let me know and I'll change it. Just wanted to give my two cents :)
There was a problem hiding this comment.
Okay, I just worry about missing this on CI. Already I have to remember to include it there in the config.
There was a problem hiding this comment.
Understandable. I can make it included by default. I'll make sure to point that out in the README as well.
There was a problem hiding this comment.
I mean, I can add to CI now if you'd like. I definitely see what you're saying. Maybe let's try it this way and see if it becomes an issue for folks later. Then we can fix.
|
I would add a I would also consider doing a Minor comments on this overall, and generally looks good. Still might want to re-review after changes, so maybe add a commit and don't squash so we can review only new changes. |
|
@joshsmith for the |
|
@green-arrow yes, so there's no specific endpoint for it, but we'll preload the roles to avoid the N+1 hit while still having their ids present in the |
|
To be clear, though, we're not |
|
Gotcha, sounds good |
green-arrow
added
awaiting review
and removed
needs submitter response
needs work
question
labels
|
@joshsmith - I believe this is ready for review again |
|
👍 |
green-arrow
force-pushed
the
add-user-roles
branch
from
316accf to
39a6b35
Compare
Also added a tag to add to all endpoints that require secret env variables be ignored in tests by default. To run all tests, including those which interface with secret env vars, run 'mix test --include requires_env' Also added user_role_view_test, user_view_test, and preloaded roles relationship on all user endpoints. Endpoint issue: #40 Contributes to #109 (exclude tests that rely on ENV vars)
green-arrow
force-pushed
the
add-user-roles
branch
from
39a6b35 to
0409bfa
Compare
Also added a tag to add to all endpoints that require secret env variables be ignored in tests by default. To run all tests, including those which interface with secret env vars, run 'mix test --include requires_env' Also added user_role_view_test, user_view_test, and preloaded roles relationship on all user endpoints. Endpoint issue: #40 Contributes to #109 (exclude tests that rely on ENV vars)
2 participants
Also added a tag to add to all endpoints that require interaction with AWS S3 that are ignored in tests by default.
To run all tests, including those which interface with S3, run
mix test --include s3Endpoint issue: #40
Contributes to #109 (exclude tests that rely on ENV vars)